Conversation
gerrycampion
changed the title
…e-evaluation-errors-that-appear-in-the-logs
…dataset confusion
…differing datatypes
| dataset = self.data_service.get_dataset(dataset_meta.filename) | ||
| referenced_datasets[dataset_meta.name] = dataset | ||
| for dataset_metadata in self.data_service.get_datasets(): | ||
| dataset = self.data_service.get_dataset(dataset_metadata.name) |
There was a problem hiding this comment.
I believe this needs to stay as
dataset = self.data_service.get_dataset(dataset_meta.filename)
Looking at the interface--i think it is another case of name and filename getting mixed. It needs the file extension, otherwise it returns an empty dataframe. I tested this on cg0370 and was not getting the correct result as it was not finding the LB dataset referenced in the CO dataset (was returning nothing for the distinct operation)
we should change all the get_datasets parameters in the interface and excel/dummy/local/usdm data services to dataset_path to avoid this
There was a problem hiding this comment.
Fixed this and renamed dataset_metadata.dataset_name to data_service_identifier to make it a bit clearer. I will do more refactoring and get_datasets parameter renaming in a follow-up pr
There was a problem hiding this comment.
Nice changes to resolve several issues, organize code, and use native functionality/metadata properties over unneeded functions.
PR preserves relrec merge functionality--tested cg0602
Correctly resolves execution error vs. skip status from parent issue
Only found one issue that needs to be addressed. Please see comment
…e-evaluation-errors-that-appear-in-the-logs
| or self.dataset_metadata.unsplit_name | ||
| ) | ||
| return define_xml_reader.extract_variables_metadata( | ||
| domain_name=domain, name=self.dataset_metadata.name |
There was a problem hiding this comment.
I might be misunderstanding this but before this looked like a lookup only using domain and now we also require dataset name. Could that cause split dataset lookups to fail if in define xml is keyed by base domain?
There was a problem hiding this comment.
This was to fix an issue with CORE-001081 (see before-report) where define metadata was not being extracted for RELREC. Here is what I would expect to see for Name/Domain when doing an itemgroupdef lookup.
| Name | Domain |
|---|---|
| AE | AE |
| QSPH | QS |
| RELREC | |
| SUPPDM | DM |
I added additional fixes now so that RELREC only needs a name and not domain. This corresponds with what is in the Define-xml specification:
There was a problem hiding this comment.
Thanks for the clarification. Do you think we should a test for these define-xml lookup cases. So the expected matching behavior is clearly covered?
RamilCDISC
left a comment
RamilCDISC
left a comment
There was a problem hiding this comment.
The PR does cleanup and improvement of the code. It resolves failing rules because of define xml processing too. The validation was done by:
- Reviewing the PR for any unwanted code or comments.
- Reviewing the PR in accordance to AC.
- Validating all unit and regression testing pass.
- Ensuring relevant tests updates.
- Ensuring new behavior for define xml is covered in testing.
- Ensuring update of cache.
- Ensuring successful execution.
- Comparing the before and after reports.
- Ensuring the report shows intended updates and something that was not intended is not changed.
- Ensuring report structure.
…e-evaluation-errors-that-appear-in-the-logs
…e-evaluation-errors-that-appear-in-the-logs
SFJohnson24
deleted the
1578-resolve-existing-rule-execution-and-rule-evaluation-errors-that-appear-in-the-logs
branch
3 participants
To compare the Execution Errors before and after, see these reports:
Report before
Report after
CORE Test Suite Updates